-
Notifications
You must be signed in to change notification settings - Fork 3.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: build the Docker Driver for arm64 #9247
Conversation
20e05b7
to
d9cac9a
Compare
@tucksaun could you resolve the conflicts? |
.drone/drone.yml
Outdated
@@ -1500,6 +1500,7 @@ name: docker-driver | |||
steps: | |||
- commands: | |||
- make docker-driver-push | |||
- make docker-driver-push PLUGIN_ARCH=-arm64 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You have to change the jsonnet file uinstead. I'll generate the Drone YML then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jeschkies a friendly ping to let you know this PR has been rebased but requires your intervention for the .drone/drone.yml
generation 🙂
d9cac9a
to
4d7a281
Compare
1898680
to
a1b1ecc
Compare
@jeschkies conflicts resolved |
Is there any issue preventing the advancement of this pull request? |
there were new conflicts I just resolved. excluding those, I believe someone (@jeschkies?) has to regenerate the |
5a31800
to
f7c1227
Compare
@tucksaun I'm sorry. I've switched teams and have missed GitHub notifications.
I was about to mentioned the multi arch image. How is the arm image tagged differently? |
IIRC docker plugins does not support multi arch images (at least it did not
went I worked on this)
…On Thu 22 Aug 2024 at 21:21, Karsten Jeschkies ***@***.***> wrote:
@tucksaun <https://github.com/tucksaun> I'm sorry. I've switched teams
and have missed GitHub notifications.
I would have loved to have a unified x64 and arm64 build but apparently
Docker drivers does not support multi arch images.
I was about to mentioned the multi arch image. How is the arm image tagged
differently?
—
Reply to this email directly, view it on GitHub
<#9247 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAGUNZUMY3BU6T4BP5DTQ3DZSZB4LAVCNFSM6AAAAABLF7PFYSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMBVGU3DENRZGI>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Hi, I think your PR would be very helpful is it a lot of work to solve conflicts to this PR can be accepted? |
f7c1227
to
288105a
Compare
@francescor not much for the Dockerfile so this is done. however this is a bit different for the pipeline because Drone has been recently removed (see #14273) and it seems like the docker plugin is neither build not released anymore (I can't find any reference to it anyway). @jeschkies, this means I'm not able to update the pipeline for testing or releasing anymore 🤷 |
I see this one https://hub.docker.com/r/miacis/loki-docker-driver is related to this context? |
I have no idea |
Naming should be be |
**What this PR does / why we need it**: Add ARM64 build and release of the Docker driver in Drone pipeline **Which issue(s) this PR fixes**: Fixes grafana#5682 **Special notes for your reviewer**: I would have loved to have a unified x64 and arm64 build but apparently Docker drivers does not support multi arch images. So instead I went with tweaking the build steps to allow cross building the image for ARM64 and added the instructions to do so in `drone.yml`. **Checklist** - [x] Reviewed the [`CONTRIBUTING.md`](https://github.com/grafana/loki/blob/main/CONTRIBUTING.md) guide (**required**) - [x] Documentation added - [ ] Tests updated - [ ] Changes that require user attention or interaction to upgrade are documented in `docs/sources/upgrading/_index.md`
9d2a580
to
28805f6
Compare
@trevorwhitney this is added. But this required more changes than you probably anticipated (Docker plugins are not shipped as images...). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
awesome, thank you for tackling this! unfortunately we can't edit the vendored jsonnet directly at that will get wiped out. that lives in grafana/loki-release
, so we'll need to port the changes over there. in the process, is there any existing GitHub action for creating the docker plugin, or do we have to do it by hand? thanks!
.github/vendor/github.com/grafana/loki-release/workflows/build.libsonnet
Show resolved
Hide resolved
'build-args': 'IMAGE_TAG=${{ needs.version.outputs.version }},GOARCH=${{ steps.platform.outputs.platform_short }}', | ||
}), | ||
|
||
releaseStep('Package as Docker plugin') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there an existing github action that handles the creation of docker plugins we can use?
you're welcome 🙂
yes this why I also extracted the changes and opened grafana/loki-release#162
not that I know of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, thanks for making the loki-release
change as well
This reverts commit 71e7472.
Super! Thank you very much to everybody, expecially tucksaun of course! |
yes, indeed, big credit to @tucksaun, thanks for the contribution! |
Co-authored-by: Trevor Whitney <trevorjwhitney@gmail.com>
Hello! (I had to go back to @tucksaun 's how to nodify developers? should I open a new issue? |
@francescor yes, please create an issue with as much detail as you can provide so someone can recreate the problem. |
I 've just created the issue #15318 |
What this PR does / why we need it:
Add ARM64 build and release of the Docker driver in Drone pipeline
Which issue(s) this PR fixes:
Fixes #5682
Special notes for your reviewer:
I would have loved to have a unified x64 and arm64 build but apparently Docker drivers does not support multi arch images. So instead I went with tweaking the build steps to allow cross building the image for ARM64 and added the instructions to do so in
drone.yml
.It seems there's a drift in the images published on Docker Hub versus the one documented for use. I updated it here but this might be wrong.
Checklist
CONTRIBUTING.md
guide (required)CHANGELOG.md
updateddocs/sources/upgrading/_index.md